Show text labels in block toolbars when option is set. #26135
Conversation
tellthemachines
added
[Focus] Accessibility (a11y)
tellthemachines
requested review from
cameronvoell,
ellatrix,
etoledom,
guarani and
youknowriad
as code owners
|
Size Change: +497 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
tellthemachines
requested review from
ajitbohra,
nerrad,
ntwb and
talldan
as code owners
|
This looks super difficult! Thanks for taking it on, @tellthemachines! 🙏 Here's a quick mockup I put together to work toward in this PR.
|
I think that kind of defeats the whole point of this text label setting. Perhaps "Transform block" would be a better short label? |
I thought about that, but I also felt the name of the block was equally important. I wonder if we'd want to work in a slight redesign in this mode. Something like this?
|
|
It's important to remember that the labels will be read out by screen readers, and I'm not sure a button labeled "paragraph transform block" will make much sense to someone using a screen reader. I think this goes back to an issue I've pointed out before, where the block toolbar's transform button is serving dual function as an indicator of the current block type. This is already questionable in my opinion when icons are used, but as can be seen here, it becomes a lot more prominent when the buttons use text labels. I think the current block type label needs to be split apart from the transform button. Semantically, it must not be inside the What if we showed the current block type label in a thin strip above all the block toolbar buttons? That would give us plenty of room to display the full block title (along with the icon), without taking away (horizontal) space from any actual toolbar buttons. If we did this on both text and icon modes, we could change the transform button to use a dedicated "transform" icon, since it would no longer be acting as the block indicator. |
|
📳✅ Tested on mobile and everything seems to be working as expected |
tellthemachines
force-pushed
the
try/block-toolbar-labels
branch
from
96782c9 to
1fedc5f
Compare
|
I'm finally getting back to this, and reading through all the comments. Thanks everyone for the feedback so far!
Good point, and having the block name here may not be very helpful to a screen reader user because, in order to get to the block toolbar, they have to select the block itself first, so will already have had the block name announced at that step. It might be more useful if they access the transform button from a form controls menu, but then it will only will show up if a particular block is selected, meaning the user will have interacted with the block just before invoking the menu. The block name is important for sighted users though.
Would this be only when the text labels are enabled, or with the icons too? |
I'd say enable it for both. I think there are already a lot of cases where it's really hard to tell what block you're editing based on the icon: Columns versus Column, Buttons versus Button, the Query and Query Loop blocks, and so on. And that's not even taking into account plugin-added blocks such as a plugin Columns block versus a core Columns block, and etc. The inspector should not be relied on for this info because it's supposed to be for secondary/uncommonly-used options, not primary info like the block title. Additionally, mobile users will not be able to have the inspector open all the time due to limited screen space, so ideally, we should make info like the block title clearly visible without having it open. Additionally, if we displayed the block title on its own little strip, we could move the block icon there as well, and we could give the transform button a dedicated icon, which would be an accessibility improvement. |
|
I removed the block name from the switcher button for now, and shortened some of the labels as per @mapk's suggestion:
I'm not sure where to place the block name though, considering that the parent block button pops up just above the toolbar, which would be its most natural location:
As for moving the "Bold", "Italic" etc. into a "Formatting" dropdown, it won't be too straightforward because the same formatting toolbar is shared between the block toolbar and the little one that pops up in captions. It feels a bit weird to turn that one into a dropdown and hide all the options. Even for the block toolbar I'm not 100% sure about it, as those are pretty frequently used options. |
tellthemachines
force-pushed
the
try/block-toolbar-labels
branch
from
8656189 to
a46769d
Compare
|
I reverted changes to the alignment labels as I was running into an issue where passing |
I think there's some gnarly hacks going on here, which I hope to clean up when I get a chance to, as part of #24898. Specifically there's some child rules that aim to optically space icons, which might just mess with things on the text labels. Probably best to leave it alone for now.
👍 👍 |
|
Ok peeps, this one's ready for a final review. Many thanks to @diegohaz for fixing the issue with the alignment labels ⭐ |
|
I found this little stuffed text (Edge browser):
|
|
@draganescu I can't reproduce that locally:
That string was shortened here. Could it be an issue with your local files, or maybe aggressive browser caching? |
| @@ -104,12 +106,18 @@ export function Button( props, ref ) { | |||
| // the tooltip is not explicitly disabled. | |||
| false !== showTooltip ) ); | |||
|
|
|||
| const descriptionId = describedBy ? uniqueId() : null; | |||
There was a problem hiding this comment.
This uniqueId will change on every render, which is unnecessary.
I think it should use the useInstanceId hook.
| </VisuallyHidden> | ||
| ) } | ||
| </> | ||
| ); |
There was a problem hiding this comment.
There's some places where we assume buttons only render a single "parent" element. In the block toolbar for instance.
So when you render two buttons in the block toolbar that have descriptions, the spacing is not evenly distributed. See #39605 (comment)
Is it possible to move the visually hidden text inside the "button" or "a" directly? Would that be valid HTML? Is there any other alternative?
There was a problem hiding this comment.
It would probably be valid HTML, but that doesn't mean it would be valid a11y. Putting it inside the <button>/<a> would make the whole thing be considered part of the button label. I don't think that would be correct/desirable, but I could be mistaken.
There was a problem hiding this comment.
Yeah, the description shouldn't be inside the element being described.
Potentially we could find another workaround by adding in actual button text as we did with the global inserter.
But the visually hidden elements are absolutely positioned; I wouldn't expect them to occupy any space at all in the canvas. Could that spacing be caused by some other issue?
There was a problem hiding this comment.
Could that spacing be caused by some other issue?
We have some styles targetting last-child and first-child CSS pseudo elements causing these issues. So these addition conflicts with these styles. I'm considering exploring changes to these styles but with no guarantee of success.
There was a problem hiding this comment.
Oh I see, the problem is not VisuallyHidden but the pseudo-element we use to render the text label. In that case, we could definitely explore a similar approach to the global inserter: render actual text in the button instead of the pseudo-element.
Description
Follows on from #24304 and addresses most of remaining work for #15311 .
How has this been tested?
Tested across browsers.
Screenshots
Top toolbars are working on large and small breakpoints (small with horizontal scrollbar)
Inline toolbar gets a bit long, not sure how we can improve it:
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: